-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor SelectNext into [role=combobox] to improve a11y #1330
Conversation
I don't think this is a "weird behavior" but rather a case where the expected behavior is not clearly defined. A common native form control has several aspects to it which get read out when a screen reader describes it. In the order they are read out, these are:
The issue with When using a combobox however, there is now a clear separation between the value and the label, and both get read out in the expected order. |
My bad. Probably not the best choice of words, as my intention was actually to say it was not the "desired" behavior for the use cases in which we want to use Sorry for the confusion. |
This is now ready to review, as I have built this branch and tested it in LMS. With these changes we can go back to use |
'w-full flex justify-between', | ||
className={classnames( | ||
'focus-visible-ring transition-colors whitespace-nowrap', | ||
'w-full flex items-center justify-between gap-x-2 p-2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These classes were automatically applied by our Button
component, but we have moved to the regular button
because our own only allows to set aria-label
via title
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change to be a combobox is good. When looking at this I noticed that the SelectNext
documentation structure needs refinement:
- At the top the component is described as a presentational component. I don't think that's really accurate any more. It isn't just styling around a native
<select>
. - There is an example near the top that shows what it looks like, but there is no code view
- Underneath that is a "Working with SelectNext" section that references
SelectNext.Option
but at this point of reading through the page, there has been no explanation or demonstration of what anOption
is or what the code structure looks like.
@@ -228,6 +246,44 @@ export default function SelectNextPage() { | |||
</Library.Demo> | |||
</Library.Example> | |||
|
|||
<Library.Example title="Labelling SelectNext"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In US English, "labeling" as one "l" in the middle. See eg. https://www.w3.org/WAI/tutorials/forms/labels/.
@@ -149,6 +149,9 @@ export type SelectProps<T> = PresentationalProps & { | |||
*/ | |||
buttonId?: string; | |||
|
|||
'aria-label'?: string; | |||
'aria-labelledby'?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An issue I noticed when reading the docs, but that I don't think we should try to address in this PR:
The documentation creates some slight confusion here because at the top of the "Component API" section it links to http://localhost:4001/using-components#presentational-components-api, which states that HTML attributes are accepted. For this particular control, those attributes are not available however. I'm not sure that accepting HTML attributes as rest props makes sense because it might be unclear which element they are applied to (the container? the button? the listbox?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was a confusion on my part. SelectNextProps
extends PresentationalProps
, but that type does not mention anything about HTML attributes.
I have noticed though, that all other components extending PresentationalProps
also extend some form of JSX.HTMLAttributes<...>
, which is not the case for SelectNext.
So I don't know if PresentationalProps
should actually implicitly extend JSX.HTMLAttributes
and we should have some other type with a different name for what is currently PresentationalProps
.
7c89d48
to
eaeaf4d
Compare
I have created an issue to address the needed documentation improvements #1331 |
Codecov Report
@@ Coverage Diff @@
## main #1330 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 62 62
Lines 926 926
Branches 354 354
=========================================
Hits 926 926
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This PR refactors the
SelectNext
so that the toggle button has[role="combobox"]
, as we have seen this is the best way to make sure screen readers announce its content and the label linked to it, if any.Without this, we have seen weird behaviors in which a label linked to the button will "replace" the content of the button itself when announced by screen readers, instead of combining both.
Additionally, and in order to make sure
SelectNext
is always labelled, this PR addsaria-label
andaria-labelledby
as optional props.This allows for the control to be labelled even in contexts where adding a
<label />
is not possible or not desired.In order to test these changes, all examples in the pattern library are now labelled, and there's a new section explaining the different ways to label
SelectNext
.